Skip to content

A composer: F1 opens a full editor for writing a post - #25

Merged
HarryCordewener merged 4 commits into
mainfrom
feat/compose-window
Aug 11, 2026
Merged

A composer: F1 opens a full editor for writing a post#25
HarryCordewener merged 4 commits into
mainfrom
feat/compose-window

Conversation

@HarryCordewener

@HarryCordewener HarryCordewener commented Aug 11, 2026

Copy link
Copy Markdown
Member

The command line grows and wraps, but ⏎ sends — so anything longer than a couple of sentences gets written somewhere else and pasted in. F1 now opens a full-screen editor and sends the result as one command.

This is the one place MultilineEditControl is right. CLAUDE.md rules it out for the command line precisely because ⏎ there has to send rather than insert; a composer is the opposite case, so undo, find, selection, mouse and a caret over wrapped rows come free instead of being written again.

What it does

  • The buffer is the whole command, verb and all — +bbpost 12=Title… — with its line breaks joined by %r. A MUSH stores a post as one string and renders the breaks itself, which is what board and mail bodies want. Blank rows at the ends are where the caret was left and are dropped; interior ones are paragraph breaks and survive as %r%r.
  • ⌥L switches escaping. literal escapes % [ ] { } ; \ so the post shows what was typed; as typed escapes nothing, so a deliberate ansi() or %r still reaches the game. The escaping runs per line before the breaks are joined in — the other order produces %%r, which posts the characters "%r" instead of a line break on every line of every literal post.
  • ⌃S sends, Esc closes and keeps the draft. Drafts are per character, in memory for the life of the run — a file would be a fourth thing this client writes, a purge entry, a --help line, and somebody's unsent post in their home directory.
  • The post goes to the focused window's character, resolved the same way ⏎ is. With nothing connected the window still opens (writing while the world is down is a real thing to be doing) and refuses at the moment of sending, keeping the post.

Three things worth reviewing

  • It is modal, and that is what makes it work at all. PinFocusToArmedBar would fight an editor needing real focus for ever — except it stands down while the main window is inactive, which a modal guarantees.
  • It refuses over an open settings screen. SettingsOverlay takes paste off the driver because its screens have no focusable target, and its own remarks warn that a focusable IPasteTarget would make both paths fire. MultilineEditControl is one, so a single paste with both open would be delivered twice.
  • Closing is what stores a draft, so sending closes first and forgets after. The other order posted the text and handed it straight back the next time the window opened — caught by ASentPostIsNotStillThereNextTime.

Consequences elsewhere

F1 is claimed in MacroKeys.AppShortcuts and answered before the settings-screen lookup, so a future unclaimed F-key cannot silently open this. Claiming it takes F1 off MacroKeys.Bindable, so its verdict changed FiresTaken and one macro test that used F1 as "a free function key" moved to F12.

The footer names ⌃S, ⌥L and Esc and nothing else — deliberately no ⌃F find, because the control has a find API with no chord bound to it and this screen is held to the same honesty rule as the settings screens.

Verification

dotnet build SharpMUTerm.slnx — 0 warnings, 0 errors. All five suites run directly: Core 864, Tui 1542, Graphics 83, Scripting 42, Web 37 — 0 failed.

New views compose and compose-literal, rendered and read off the frame: header band at row 1, editor filling rows 2–31 in the screens' own edit colour, footer pinned at row 32.

Two layout notes found by looking at the frame rather than the markup: VerticalAlignment.Fill alone is not enough (the control reads arranged bounds only once arranged, so the first frame used its ten-row default), and colours must set both pairs, because this control paints from the focused pair and always has focus here.

🤖 Generated with Claude Code

https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN

Summary by CodeRabbit

  • New Features
    • Added a full-screen post composer opened with F1 or term:compose.
    • Supports multiline editing, per-character drafts, resizing, and target display.
    • Ctrl+S sends posts; Alt+L toggles typed or literal escaping.
    • Esc closes the composer while preserving non-empty drafts.
    • Prevents empty or disconnected submissions and conflicting modal openings.
  • Documentation
    • Added help and snapshot documentation for composer controls, line breaks, escaping modes, and draft behavior.
  • Tests
    • Added coverage for composition, sending, drafts, shortcuts, escaping, and connection states.

The command line is a command line: it grows and wraps, but ⏎ sends, so
anything longer than a couple of sentences is written somewhere else and
pasted in. F1 opens a full-screen editor instead — undo, find, selection,
mouse and a caret over wrapped rows — and sends the result as one command.

This is the one place `MultilineEditControl` is right. It is ruled out for
the command line precisely because ⏎ there has to send rather than insert;
a composer is the opposite case, so none of that behaviour is written again.

What is sent (`ComposeMessage`, Core): the buffer is the whole command,
verb and all, with its line breaks joined by `%r` — a MUSH stores a post
as one string and renders the breaks itself, which is what `+bbpost` and
`@mail` bodies want. Blank rows at the ends are where the caret was left
and are dropped; interior ones are paragraph breaks and survive as `%r%r`.

⌥L switches escaping. In `literal` the body's `% [ ] { } ;` and `\` are
escaped so the post shows what was typed; in `as typed` nothing is. The
escaping runs per line *before* the breaks are joined in, so the `%r` this
writes is never itself escaped — the other order produces `%%r`, which
posts the characters "%r" instead of a line break on every line.

Modal, which is what makes it possible: `PinFocusToArmedBar` would fight an
editor needing real focus for ever, and it stands down while the main window
is inactive. It refuses over an open settings screen — that overlay takes
paste off the driver because its screens have no focusable target, and this
one is exactly such a target, so both would fire on a single paste.

Drafts are per character, in memory for the life of the run. Keyed by the
window's owner, never `_active`. Closing raises the event that stores one,
so sending closes first and forgets after.

F1 is claimed in MacroKeys.AppShortcuts and answered before the settings
screen lookup, so a future unclaimed F-key cannot silently open this. That
takes F1 off MacroKeys.Bindable, which moved one macro test to F12.

New views `compose` and `compose-literal`. Build clean, 0 warnings; Core
864, Tui 1542, Graphics 83, Scripting 42, Web 37 — all passing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8b41449f-6e50-4d62-b8f0-526c190a174d

📥 Commits

Reviewing files that changed from the base of the PR and between ecbe0a6 and dc1d50a.

📒 Files selected for processing (5)
  • CLAUDE.md
  • src/SharpMUTerm.Tui/Program.cs
  • src/SharpMUTerm.Tui/SharpMUTermApp.cs
  • tests/SharpMUTerm.Tui.Tests/ComposeWindowTests.cs
  • tests/SharpMUTerm.Tui.Tests/SnapshotViewEmptyConfigTests.cs

Walkthrough

Adds a full-screen F1 post composer. It converts multiline text into MUSH commands, supports literal escaping, preserves per-character drafts, integrates with term:compose, and adds application, snapshot, and end-to-end tests.

Changes

Post composer

Layer / File(s) Summary
Message composition contract
src/SharpMUTerm.Core/Commands/ComposeMessage.cs, src/SharpMUTerm.Core/Commands/CommandCatalog.cs, tests/SharpMUTerm.Core.Tests/Commands/ComposeMessageTests.cs
Adds newline normalization, boundary trimming, literal escaping, ComposeEscaping, the term:compose catalog entry, and core tests.
Composer modal
src/SharpMUTerm.Tui/ComposeOverlay.cs
Adds the multiline editor, keyboard controls, headers, footer controls, resizing, focus restoration, and cleanup.
Application integration
src/SharpMUTerm.Tui/SharpMUTermApp.cs, src/SharpMUTerm.Tui/MacroKeys.cs, src/SharpMUTerm.Tui/DemoScene.cs, src/SharpMUTerm.Tui/Program.cs, CLAUDE.md
Wires F1 and term:compose, sends composed commands, preserves drafts per character, and documents the editor and snapshot views.
Composer validation
tests/SharpMUTerm.Tui.Tests/ComposeWindowTests.cs, tests/SharpMUTerm.Tui.Tests/MacroKeyCaptureTests.cs, tests/SharpMUTerm.Tui.Tests/PaneActivationTests.cs, tests/SharpMUTerm.Tui.Tests/SnapshotViewEmptyConfigTests.cs
Validates opening, sending, escaping, drafts, session routing, shortcut ownership, disconnected states, modal conflicts, quit handling, and snapshot views.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant SharpMUTermApp
  participant ComposeOverlay
  participant ComposeMessage
  participant Session
  User->>SharpMUTermApp: Press F1 or invoke term:compose
  SharpMUTermApp->>ComposeOverlay: Open for focused character
  User->>ComposeOverlay: Enter body and press Ctrl+S
  ComposeOverlay->>SharpMUTermApp: Emit body and ComposeEscaping mode
  SharpMUTermApp->>ComposeMessage: Build composed command
  ComposeMessage-->>SharpMUTermApp: Return command or null
  SharpMUTermApp->>Session: Send command
Loading

Possibly related PRs

Suggested reviewers: claude

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 62.22% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: an F1-opened full-screen composer for writing posts.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/SharpMUTerm.Tui/SharpMUTermApp.cs`:
- Around line 2950-2963: Update ToggleComposer and the shared AnyOverlayOpen
guard to prevent modal overlays from stacking: include _composer.IsOpen in
AnyOverlayOpen, and use that aggregate when opening the palette, quit prompt,
message log, and settings screens. Preserve the existing refusal behavior while
ensuring each listed overlay entry point rejects opening when any overlay is
already active.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: b92c6941-eef4-4e19-8396-3ee0ea912727

📥 Commits

Reviewing files that changed from the base of the PR and between 79fd7ec and 6ad25ca.

📒 Files selected for processing (13)
  • CLAUDE.md
  • src/SharpMUTerm.Core/Commands/CommandCatalog.cs
  • src/SharpMUTerm.Core/Commands/ComposeMessage.cs
  • src/SharpMUTerm.Tui/ComposeOverlay.cs
  • src/SharpMUTerm.Tui/DemoScene.cs
  • src/SharpMUTerm.Tui/MacroKeys.cs
  • src/SharpMUTerm.Tui/Program.cs
  • src/SharpMUTerm.Tui/SharpMUTermApp.cs
  • tests/SharpMUTerm.Core.Tests/Commands/ComposeMessageTests.cs
  • tests/SharpMUTerm.Tui.Tests/ComposeWindowTests.cs
  • tests/SharpMUTerm.Tui.Tests/MacroKeyCaptureTests.cs
  • tests/SharpMUTerm.Tui.Tests/PaneActivationTests.cs
  • tests/SharpMUTerm.Tui.Tests/SnapshotViewEmptyConfigTests.cs

Comment thread src/SharpMUTerm.Tui/SharpMUTermApp.cs
`AnyOverlayOpen` is this app's statement that a modal surface owns the
screen, and the composer was missing from it — so ⌃R and the other guards
that read that flag acted while a post was being written. It is in the
list now.

F1 correspondingly refuses over *any* surface rather than only over a
settings screen. That guard was written for the paste hazard specifically
(SettingsOverlay listens at the driver because its screens have no
focusable target, and the editor is one), but the rule it was enforcing was
always the general one. The refusal names the surface in the way rather
than saying "a surface", like every other refusal here.

Not changed: the palette, quit prompt, message log and settings screens
still open over each other, which they did before this branch. Making them
mutually exclusive is a behaviour change to five existing surfaces with its
own test surface, and ⌃P has to keep closing the palette it opened.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
@HarryCordewener

Copy link
Copy Markdown
Member Author

Addressed in ac5d45a, partly.

Taken: _composer.IsOpen is in AnyOverlayOpen now — you're right that it belongs there. That property is this app's statement that a modal surface owns the screen, and with the composer missing from it ⌃R and the other guards that read the flag acted while a post was being written. F1 correspondingly refuses over any surface rather than only a settings screen; the guard was written for the paste hazard specifically (SettingsOverlay listens at the driver because its screens have no focusable target, and MultilineEditControl is one), but the rule behind it was always the general one. The refusal names the surface that is in the way rather than saying "a surface". Two tests added.

Not taken: applying the aggregate to the palette, quit prompt, message log and settings screens. Those already open over each other on main — it is not something this branch introduced — and making them mutually exclusive is a behaviour change across five surfaces with its own test surface, plus ToggleMenu has to keep closing the palette it opened, so it cannot simply guard on AnyOverlayOpen. Worth doing, but as its own change rather than folded into the composer.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
tests/SharpMUTerm.Tui.Tests/ComposeWindowTests.cs (1)

223-235: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Dispose the application fixtures and restore Console.In.

The disconnected test and both helpers create SharpMUTermApp instances and set Console.In to TextReader.Null. No test disposes the app or restores the process-global input stream. SharpMUTermApp.DisposeAsync owns sessions, timers, fetchers, the image loader, and diagnostics, so this can leak state across tests.

Use await using fixtures that forward to SharpMUTermApp.DisposeAsync, and restore the previous Console.In during cleanup.

Also applies to: 294-334

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/SharpMUTerm.Tui.Tests/ComposeWindowTests.cs` around lines 223 - 235,
Update the disconnected test and both helper fixtures that create SharpMUTermApp
instances to use await using, ensuring SharpMUTermApp.DisposeAsync runs during
cleanup. Capture the previous Console.In before assigning TextReader.Null and
restore it in each fixture’s cleanup, including all paths.
src/SharpMUTerm.Tui/SharpMUTermApp.cs (2)

2990-3017: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Record composed posts in command history.

OnCommandEntered adds normal commands to HistoryFor(bar) before sending. SendComposed calls the session directly, so successful F1 posts are absent from ↑/↓ and ⌃R history.

Route the composed command through shared history bookkeeping with the same credential filter before sending.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/SharpMUTerm.Tui/SharpMUTermApp.cs` around lines 2990 - 3017, Update
SendComposed to record the successfully built composed line through the same
history bookkeeping used by OnCommandEntered, including its credential filter,
before sending it. Preserve the existing refusal behavior and only add the entry
after ComposeMessage.Build succeeds; then continue dispatching the line through
the session.

2973-3017: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Preserve the composer draft until a connected send succeeds.

Lines [2973-2977] treat an existing WorldSession as sendable. A session can exist while IsConnected is false. Lines [3015-3017] close the composer, remove _composeDrafts, and start SendUserInputAsync without observing its result. A disconnected session or a later transport failure can lose the post.

Pass the connection state to canSend, reject disconnected sessions in SendComposed, and retain the draft until the send completes successfully.

Minimum state guard
-            canSend: session is not null);
+            canSend: session?.IsConnected == true);

-        if (WindowSession(windowId) is not { } session)
+        if (WindowSession(windowId) is not { IsConnected: true } session)

This guard is required in addition to delaying draft removal until the asynchronous send succeeds.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/SharpMUTerm.Tui/SharpMUTermApp.cs` around lines 2973 - 3017, Update the
composer opening flow to pass session.IsConnected to canSend, and make
SendComposed reject sessions that are null or disconnected via RefuseCommand
without closing the composer. Await session.SendUserInputAsync(line), and only
after it succeeds close the composer and remove _composeDrafts; preserve the
draft when sending fails or the transport is disconnected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/SharpMUTerm.Tui/SharpMUTermApp.cs`:
- Around line 3191-3207: Apply the AnyOverlayOpen guard to every modal-opening
path, including settings, quit prompt, command palette, and client message
viewer entry points. Update the corresponding handlers or centralize modal
activation so these actions refuse to open whenever any overlay, including the
composer, is already open; preserve existing behavior when no overlay is active.

---

Outside diff comments:
In `@src/SharpMUTerm.Tui/SharpMUTermApp.cs`:
- Around line 2990-3017: Update SendComposed to record the successfully built
composed line through the same history bookkeeping used by OnCommandEntered,
including its credential filter, before sending it. Preserve the existing
refusal behavior and only add the entry after ComposeMessage.Build succeeds;
then continue dispatching the line through the session.
- Around line 2973-3017: Update the composer opening flow to pass
session.IsConnected to canSend, and make SendComposed reject sessions that are
null or disconnected via RefuseCommand without closing the composer. Await
session.SendUserInputAsync(line), and only after it succeeds close the composer
and remove _composeDrafts; preserve the draft when sending fails or the
transport is disconnected.

In `@tests/SharpMUTerm.Tui.Tests/ComposeWindowTests.cs`:
- Around line 223-235: Update the disconnected test and both helper fixtures
that create SharpMUTermApp instances to use await using, ensuring
SharpMUTermApp.DisposeAsync runs during cleanup. Capture the previous Console.In
before assigning TextReader.Null and restore it in each fixture’s cleanup,
including all paths.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9bd4c65e-c2af-4661-968f-ca5e0edd567f

📥 Commits

Reviewing files that changed from the base of the PR and between 6ad25ca and ac5d45a.

📒 Files selected for processing (2)
  • src/SharpMUTerm.Tui/SharpMUTermApp.cs
  • tests/SharpMUTerm.Tui.Tests/ComposeWindowTests.cs

Comment on lines 3191 to +3207
private bool AnyOverlayOpen =>
_palette.IsOpen || _settings.IsOpen || _quit.IsOpen || _messageLog.IsOpen || _historySearch.IsOpen
|| _prefixPanel.IsOpen;
|| _prefixPanel.IsOpen || _composer.IsOpen;

/// <summary>
/// What <see cref="AnyOverlayOpen"/> is currently true because of, in the words the reader knows the
/// surface by. A refusal that named "a surface" would leave somebody looking for which one; every
/// other refusal in this client names the thing it is talking about, and this is one string.
/// </summary>
private string OpenOverlayName() =>
_settings.IsOpen ? "the settings screen"
: _palette.IsOpen ? "the command surface"
: _quit.IsOpen ? "the quit prompt"
: _messageLog.IsOpen ? "the client messages"
: _historySearch.IsOpen ? "the history search"
: _prefixPanel.IsOpen ? "the pane keys panel"
: "what is open";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major

Apply AnyOverlayOpen to every modal entry point.

Line [3193] includes the composer in the aggregate, but the aggregate is not used when opening settings at Lines [4465-4471], the quit prompt at Line [4527], the command palette at Lines [6080-6083], or the client message viewer at Lines [5774-5776]. Global shortcuts run before HandleWindowKey at Lines [7339-7354], so these actions can stack another surface over the composer.

Guard each open path or centralize modal activation. This is the same unresolved stacking issue reported in the previous review.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/SharpMUTerm.Tui/SharpMUTermApp.cs` around lines 3191 - 3207, Apply the
AnyOverlayOpen guard to every modal-opening path, including settings, quit
prompt, command palette, and client message viewer entry points. Update the
corresponding handlers or centralize modal activation so these actions refuse to
open whenever any overlay, including the composer, is already open; preserve
existing behavior when no overlay is active.

The guard was one-sided: the composer refused to open over a surface, and
every surface still opened over the composer, because a global shortcut
runs before any window sees the key. ⌃P, F2–F9 and ⌃P ▸ Show client
messages now refuse while a post is being written, naming what is in the
way and saying that Esc keeps the post.

⌃Q is deliberately not one of them. A client you cannot leave from a modal
is worse than one that stacks a prompt, so the quit prompt still opens —
and QuitFactsNow counts the unsent post instead, which is the answer that
helps: it says what leaving would cost. The composer keeps drafts in memory
only, so quitting is precisely the thing that loses them. The open window's
buffer is counted separately from the stored drafts because it is not in
that dictionary until it closes.

Still not changed: the pre-existing surfaces open over each other as they
did before this branch.

Core 864, Tui 1546, Graphics 83, Scripting 42, Web 37 — all passing, build
clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
@HarryCordewener

Copy link
Copy Markdown
Member Author

Taken, in ecbe0a6 — you were right that the guard was one-sided. ⌃P, F2–F9 and ⌃P ▸ Show client messages now refuse while a post is being written, naming what is in the way and saying Esc keeps the post. NoOtherSurfaceOpensOverTheComposer drives all three through the real chords, since a global shortcut running ahead of the window is exactly how the second modal got on top.

⌃Q is deliberately excluded. A client you cannot leave from a modal is worse than one that stacks a prompt, so the quit prompt still opens — and QuitFactsNow now counts the unsent post instead, which is the answer that actually helps: it says what leaving would cost. The composer keeps drafts in memory only, so quitting is precisely the thing that loses them. The open window's buffer is counted separately from the stored drafts, because it is not in that dictionary until it closes.

Still not changed, for the reason given before: the pre-existing surfaces open over each other exactly as they did before this branch. That is a five-surface behaviour change with its own test surface, and ⌃P has to keep closing the palette it opened, so it cannot simply read AnyOverlayOpen.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/SharpMUTerm.Tui/SharpMUTermApp.cs`:
- Around line 3200-3216: The XML documentation around the overlay guard contains
an unclosed outer summary element. Update the documentation preceding the
visible nested summary so the summary opened around line 3195 is closed before
the nested documentation begins, or merge the descriptions into a single
properly closed summary while preserving the existing content.
- Around line 4610-4611: The draft count in SharpMUTermApp’s posts calculation
double-counts the active reopened composer; exclude its character key from
_composeDrafts while _composer.IsOpen, preserving the existing open-body count.
Add a regression test in tests/SharpMUTerm.Tui.Tests/ComposeWindowTests.cs at
lines 321-330 that saves, reopens, and quits a post, then asserts one unsent
draft.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3471feb4-3305-453e-a9b4-cdc01d0222c7

📥 Commits

Reviewing files that changed from the base of the PR and between ac5d45a and ecbe0a6.

📒 Files selected for processing (2)
  • src/SharpMUTerm.Tui/SharpMUTermApp.cs
  • tests/SharpMUTerm.Tui.Tests/ComposeWindowTests.cs

Comment on lines +3200 to +3216
/// <summary>
/// Refuses a surface that would open <em>over</em> the composer, naming what is in the way. The
/// composer's own guard is the other half of this; together they make the pair mutually exclusive
/// rather than one-sided, which is the whole of the stacking rule for this feature.
/// <para>
/// <b>⌃Q is deliberately not one of the callers.</b> Quitting has to work from wherever the reader
/// is — a modal that could refuse it would be a client you cannot leave — so the quit prompt opens
/// over the composer and <see cref="QuitFactsNow"/> counts the unsent post instead, which is the
/// answer that actually helps: it says what would be lost rather than declining to ask.
/// </para>
/// <para>
/// The pre-existing surfaces still open over <em>each other</em>, as they did before the composer
/// existed. Making all of them mutually exclusive is a change to five surfaces with its own test
/// surface — and ⌃P has to keep closing the palette it opened, so it cannot simply read
/// <see cref="AnyOverlayOpen"/> — which is a separate piece of work rather than part of this one.
/// </para>
/// </summary>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Close the preceding XML documentation element.

Line 3200 starts a nested <summary> element. The <summary> opened on Line 3195 is never closed. This produces malformed XML documentation and can add compiler documentation warnings.

Close the first summary before Line 3200, or merge both descriptions into one summary.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/SharpMUTerm.Tui/SharpMUTermApp.cs` around lines 3200 - 3216, The XML
documentation around the overlay guard contains an unclosed outer summary
element. Update the documentation preceding the visible nested summary so the
summary opened around line 3195 is closed before the nested documentation
begins, or merge the descriptions into a single properly closed summary while
preserving the existing content.

Comment thread src/SharpMUTerm.Tui/SharpMUTermApp.cs Outdated
Both conflicts were "keep both". `--view compose`/`compose-literal` and
`--view links` are separate arms of the same snapshot switch; the view list
and CLAUDE.md each gain both sections rather than one replacing the other.

Also fixes two review items on the way past:

- `OpenOverlayName`'s doc comment had been stranded above
  `ComposerIsInTheWay` when that method was inserted, so it documented the
  wrong member and the other had none.

- ⌃Q counted a reopened post twice. After Esc and F1 the same text is in
  the draft store *and* in the open window — the store kept it on the way
  out and the window is holding it again — so the prompt said "2 unsent
  drafts" for one post. The open composer's own character is now excluded
  from the stored count.

Core 883, Tui 1601, Graphics 83, Scripting 42, Web 37 — all passing, build
clean. compose, links and tint-input all still render.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
@HarryCordewener

Copy link
Copy Markdown
Member Author

Merged main in (dc1d50a) — #23 and #24 landing is what made this conflict. Both conflicts were keep both: --view compose/compose-literal and --view links are separate arms of the same snapshot switch, and the view list and CLAUDE.md each gain both sections. compose, links and tint-input all still render.

Two of the three new items are fixed in the same commit:

Doc comment — correct, though the mechanism was slightly different from the diagnosis: OpenOverlayName's summary got stranded above ComposerIsInTheWay when that method was inserted between the comment and its member, so one method carried the wrong documentation and the other had none. Reattached.

Double-counted post — real bug, thank you. After Esc and F1 the same text is in _composeDrafts and in the open window (the store kept it on the way out, the window is holding it again), so ⌃Q said "2 unsent drafts" for one post. The open composer's own character key is now excluded from the stored count, and AReopenedPostIsCountedOnce saves, reopens, quits and asserts 1 unsent draft with an explicit DoesNotContain("2 unsent draft").

The stacking Major was already addressed in ecbe0a6, which landed after that thread was posted: ⌃P, F2–F9 and ⌃P ▸ Show client messages refuse while a post is being written (NoOtherSurfaceOpensOverTheComposer drives all three through the real chords), and ⌃Q is deliberately excluded in favour of counting the post — a client you cannot leave from a modal is worse than one that stacks a prompt.

Post-merge: build clean, 0 warnings. Core 883, Tui 1601, Graphics 83, Scripting 42, Web 37 — 0 failed.

@HarryCordewener
HarryCordewener merged commit e8b42e8 into main Aug 11, 2026
3 checks passed
@HarryCordewener
HarryCordewener deleted the feat/compose-window branch August 11, 2026 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant